Improve Prebid auction diagnostics#671
Conversation
SummaryRicher Blocking🔧 wrench
Non-blocking🤔 thinking
♻️ refactor
🌱 seedling
📌 out of scope
⛏ nitpick
CI Status
|
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
The diagnostic improvements are well-scoped and the prior round's findings are cleanly addressed (current-context truncation, debug-gated body_preview, PREBID_INTEGRATION_ID constant, regression tests for the 500/1000-char caps and attached-detail suppression). CI is green and the new tests pin the right boundaries.
The remaining concerns are about consistency of that boundary: a few error paths slip past the debug gate that the 4xx path now establishes. Inline comments below cover the three specific call sites; cross-cutting items are listed here.
Blocking
🔧 wrench
- Parse-error message bypasses debug gate:
parse_responsereturns serde-error detail + body byte length unconditionally, while the 4xx branch hides body content behindconfig.debug(crates/trusted-server-core/src/integrations/prebid.rs:1141). warn!emits up to 1000 chars of upstream body without debug gate: previously trace-only; this is a logging behavior change worth gating in line with the metadata-path treatment (crates/trusted-server-core/src/integrations/prebid.rs:1112).- Launch-failure metadata exposes internal
messagestrings: e.g. signing-key kid viaRequestSigner::from_services().provider_error_messageis sound; the issue is at the call site choosing to emitcurrent_context()verbatim forerror_type=launch_failed(crates/trusted-server-core/src/auction/orchestrator.rs:385).
Non-blocking
🌱 seedling
- Network-layer
select()failures still drop providers fromprovider_responses(crates/trusted-server-core/src/auction/orchestrator.rs:481-486). The PR adds metadata forlaunch_failed,parse_response, andunsupported_response_body, but transport-level errors emerging fromselect()Err — and providers still pending after the deadline (backend_to_providerleftovers) — produce no entry./auctionclients can't distinguish "no bid" from "transport error" for those. Pre-existing, but the PR's stated intent is unified observability so it's a natural follow-up.
📌 out of scope
- JS bundle still defaults to bundling the Rubicon adapter (
crates/js/lib/build-all.mjs:32). Withclient_side_bidders = []now the default intrusted-server.toml, the bundled Rubicon Prebid.js adapter is dead weight unless a publisher overrides the list. Either defaultTSJS_PREBID_ADAPTERS=''to match, or havebuild-all.mjsderive the adapter list from the merged TOML. Worth a follow-up issue.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests: PASS
- integration / browser tests: PASS
- CodeQL / format-typescript / format-docs: PASS
ChristianPavilonis
requested review from
aram356 and
prk-Jr
and removed request for
prk-Jr
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The remaining review concern is in the Prebid error-body preview path. The public preview is capped, but the helper still performs uncapped UTF-8/lossy processing before the cap is applied, and it is invoked even when debug mode is off.
Blocking
🔧 wrench
- Uncapped preview work before truncation:
prebid_body_previewconverts the entire upstream body withString::from_utf8_lossy(body)before taking the first 1000 chars, and the non-success response path calls it before checkingconfig.debug. Large or invalid UTF-8 error bodies can still drive uncapped validation/allocation even when no preview will be exposed. See inline.
CI Status
- GitHub checks: Analyze (javascript-typescript) PASS
- Local fmt: PASS (
cargo fmt --all -- --check) - Local targeted Rust tests: PASS (
prebid_body_preview;provider_error_response)
ChristianPavilonis
force-pushed
the
feature/prebid-auction-diagnostics
branch
from
25eaca6 to
cedc9cf
Compare
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Deep re-review against main at HEAD 50e3000d. Two prior approvals (aram356, prk-Jr) and all CI green. The PR has iterated through three rounds of reviewer feedback and addressed every prior finding (Debug→Display, debug-gated body previews, static launch-failure messages, PREBID_INTEGRATION_ID constant, byte-bounded from_utf8_lossy, dedicated truncation tests). The remaining findings below are forward-looking invariants and one cross-cutting concern; none block correctness.
Blocking
🔧 wrench
- SECURITY invariant comment on
provider_error_message: safe today because allchange_contextsites use static or operator-controlled message strings, but a future provider interpolating upstream-controlled content intoTrustedServerError::*.messagewould silently undo this PR's security work. See inline atcrates/trusted-server-core/src/auction/orchestrator.rs:25. - Lift
error_typestrings to constants:"http_status"/"parse_response"/"unsupported_response_body"/"launch_failed"/"empty_response"are now an implicit public contract — promote to module constants to prevent typos and centralize the taxonomy. See inline atcrates/trusted-server-core/src/auction/orchestrator.rs:41.
Non-blocking
🌱 seedling
- Opt-in for provider diagnostics exposure:
error_type/message/http_status/body_previeware now unconditionally surfaced on/auction. Some production operators may prefer server-logs-only diagnostics and a minimal public response. Worth a follow-upexpose_provider_diagnostics: boolflag inAuctionConfig(defaulttruefor backward compat). Out of scope for this PR.
📝 note
- No-bid taxonomy shift: a 200 OK response with empty body previously failed
serde_json::from_slice(&[])and surfaced as a provider error; now it isBidStatus::NoBidwithreason = "empty_response". This will shift the error-rate ↔ no-bid-rate ratio in any dashboards comparing pre/post merge. Worth a one-line release note.
⛏ nitpick
- UTF-8 boundary test gap:
prebid_body_preview_ignores_bytes_after_bounded_sliceexercises the byte cap with ASCII input only; the adversarial case is a multi-byte character straddling the byte boundary. See inline atcrates/trusted-server-core/src/integrations/prebid.rs:3202.
CI Status
- fmt: PASS
- clippy: PASS
- cargo test: PASS
- vitest: PASS
- browser/integration: PASS
Locally verified cargo test -p trusted-server-core --lib against the touched test names (all 12 new tests pass) and cargo clippy -p trusted-server-core --all-targets --all-features -- -D warnings (clean).
| .chars() | ||
| .take(PROVIDER_ERROR_MESSAGE_CHARS) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
🔧 wrench — Forward-looking exposure invariant should be documented inline.
current_context().to_string() is safe today: every change_context site in the orchestrator uses static or operator-controlled message strings, and platform_response_to_fastly's only error is a hard-coded "streaming platform response body is not supported by Fastly response conversion". But this is now an implicit security invariant — a future provider whose parse_response does
.change_context(TrustedServerError::X { message: format!("Failed: {upstream_body}") })would silently route upstream_body into provider_details[*].metadata.message on /auction, undoing the work this PR just did.
Fix — add a // SECURITY: block above provider_error_message:
// SECURITY: the returned string is included verbatim (truncated to
// PROVIDER_ERROR_MESSAGE_CHARS) in the public /auction response via
// ProviderSummary.metadata["message"]. Providers MUST NOT interpolate
// upstream-controlled content (response bodies, parse errors, headers)
// into their TrustedServerError::*.message fields. Use static text and
// log details server-side with `log::warn!` instead.
fn provider_error_message(error: &Report<TrustedServerError>) -> String {| fn provider_launch_failed_response(provider_name: &str, response_time_ms: u64) -> AuctionResponse { | ||
| AuctionResponse::error(provider_name, response_time_ms) | ||
| .with_metadata("error_type", serde_json::json!("launch_failed")) | ||
| .with_metadata("message", serde_json::json!("Provider launch failed")) |
There was a problem hiding this comment.
🔧 wrench — Lift error_type strings to constants.
The values "http_status", "parse_response", "unsupported_response_body", "launch_failed" are an implicit public contract with downstream consumers (dashboards, alerting, mediator logic). Keeping them as string literals at four call sites is a typo risk and lacks discoverability.
Fix — pull them up next to PROVIDER_ERROR_MESSAGE_CHARS:
const PROVIDER_ERROR_MESSAGE_CHARS: usize = 500;
const ERROR_TYPE_HTTP_STATUS: &str = "http_status";
const ERROR_TYPE_PARSE_RESPONSE: &str = "parse_response";
const ERROR_TYPE_UNSUPPORTED_BODY: &str = "unsupported_response_body";
const ERROR_TYPE_LAUNCH_FAILED: &str = "launch_failed";Then replace the literals at orchestrator.rs:40, 458, 473 and the prebid.rs "http_status" site at prebid.rs:1509. Same idea applies to the "empty_response" reason literal in prebid.rs:1530.
| !preview.contains('\u{fffd}') && !preview.contains("tail"), | ||
| "should not process bytes beyond the bounded preview slice" | ||
| ); | ||
| } |
There was a problem hiding this comment.
⛏ nitpick — UTF-8 boundary test gap.
prebid_body_preview_ignores_bytes_after_bounded_slice exercises the byte cap with ASCII input (b'x'). The negative assertion !preview.contains('\u{fffd}') only passes because no multi-byte character straddles the byte boundary.
The real adversarial case is a 3- or 4-byte UTF-8 character whose first byte lands at PREBID_ERROR_BODY_PREVIEW_BYTES - 1. from_utf8_lossy should substitute U+FFFD at the boundary instead of leaking the next bytes. Worth a small additional test:
#[test]
fn prebid_body_preview_replaces_partial_utf8_at_boundary() {
let mut body = vec![b'a'; PREBID_ERROR_BODY_PREVIEW_BYTES - 1];
// Three-byte U+2603 SNOWMAN straddles the cap by 2 bytes.
body.extend_from_slice("\u{2603}".as_bytes());
body.extend_from_slice(b"tail");
let preview = prebid_body_preview(&body);
assert!(
preview.ends_with('\u{fffd}'),
"partial multibyte char at byte cap should become U+FFFD, not leak following bytes"
);
assert!(
!preview.contains("tail"),
"should not include bytes beyond the bounded slice"
);
}3 participants
Summary
/auctionprovider metadata, with capped body previews included only when Prebid debug mode is enabled.204 No Contentor empty200) asnobidinstead of providererror.client_side_bidderssample config so it is not enabled by default.Changes
crates/trusted-server-core/src/integrations/prebid.rsdebug, treats empty successful responses as no-bid, usesPREBID_INTEGRATION_IDfor provider response IDs, and adds tests for truncation/non-UTF-8 preview behavior.crates/trusted-server-core/src/auction/orchestrator.rstrusted-server.tomlclient_side_biddersfrom["rubicon"]to[].Compatibility notes
provider_detailsmay include launch-failure diagnostics before awaited provider responses. Bid selection is order-insensitive, but consumers should not rely onprovider_detailsbeing ordered by response completion.Closes
N/A — no issue filed.
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo test -p trusted-server-core prebid -- --nocapture;cargo test -p trusted-server-core auction -- --nocapture; targeted review-feedback tests forprovider_error,prebid_body_preview, andparse_response_non_successChecklist
unwrap()in production code — useexpect("should ...")logmacros per CLAUDE.md (template saystracing, but this repo standard islog)